-
Notifications
You must be signed in to change notification settings - Fork 2.2k
tests/int: cleanup #4945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests/int: cleanup #4945
Conversation
1. Remove the devicemapper driver mentions, and is it no longer supported by docker (or podman). 2. Remove the test example -- we have plenty of real ones. 3. Add a link to (well written and extensive) bats documentation. 4. Fix capitalization in a sentence. Signed-off-by: Kir Kolyshkin <[email protected]>
This is a bit opinionated, but some comments in integration tests do not really help to understand the nature of the tests being performed by stating something very obvious, like # run busybox detached runc run -d busybox To make things worse, these not-so-helpful messages are being copy/pasted over and over, and that is the main reason to remove them. Signed-off-by: Kir Kolyshkin <[email protected]>
Move the repetitive code and comment into setup_netns. Signed-off-by: Kir Kolyshkin <[email protected]>
...instead of an explicit or absent status check. Signed-off-by: Kir Kolyshkin <[email protected]>
Commands that are not run via "run" helper (cat, mkdir, __runc) do not set $status, so it makes no sense to check it. Fixes: 94505a0, ed54837 Signed-off-by: Kir Kolyshkin <[email protected]>
This misleading comment is obviously a copy/paste from the previous test. Fix it. Fixes: dd69623 Signed-off-by: Kir Kolyshkin <[email protected]>
In our bats tests, runc itself is a wrapper which calls bats run helper, so using "run runc" is wrong as it results in calling run helper twice. Fixes: 8d180e9 Signed-off-by: Kir Kolyshkin <[email protected]>
The "runc delete --force [paused container]" test case does not check runc pause exit code, and if added, the test fails in rootless tests, because: - not all rootless tests have access to cgroups; - rootless containers doesn't have default cgroups path. To fix, add: - setup for rootless case; - require cgroups_freezer; - runc pause exit code check. Signed-off-by: Kir Kolyshkin <[email protected]>
This is mostly to improve readability. While at it, make the script more robust by adding -e option to shell. The exception is echo $pid which is opportunistic and may fail depending on the order of pids in the file. Also, remove the empty comment and a shellcheck annotation. Fixes: c91fe9a Signed-off-by: Kir Kolyshkin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR performs cleanup and refactoring of the integration tests, removing unnecessary comments, fixing minor bugs, and modernizing test patterns. The changes improve code maintainability without altering test functionality.
Key changes:
- Removed obvious/redundant comments like "run busybox detached" and "check state"
- Converted old-style
run+[ "$status" -eq 0 ]patterns to the more conciserun -0orrun !syntax - Fixed rootless pause test requirements and a checkpoint test using
runtwice - Refactored netdev tests to reduce duplication
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/userns.bats | Modernized status checks using run ! syntax |
| tests/integration/update.bats | Removed redundant comments and modernized run commands |
| tests/integration/umask.bats | Removed obvious comment |
| tests/integration/tty.bats | Removed redundant comments throughout |
| tests/integration/state.bats | Removed obvious comments |
| tests/integration/start_hello.bats | Removed redundant comments |
| tests/integration/start_detached.bats | Removed obvious comments |
| tests/integration/start.bats | Removed redundant comments |
| tests/integration/scheduler.bats | Modernized run command with status check |
| tests/integration/pidfd-socket.bats | Removed bogus status checks after commands |
| tests/integration/pause.bats | Removed obvious comments |
| tests/integration/netdev.bats | Refactored with setup_netns function and modernized run commands |
| tests/integration/mask.bats | Removed obvious comments |
| tests/integration/list.bats | Removed redundant comment |
| tests/integration/kill.bats | Removed obvious comments |
| tests/integration/exec.bats | Removed redundant comments |
| tests/integration/events.bats | Removed obvious comments |
| tests/integration/delete.bats | Added rootless requirements and fixed pause test |
| tests/integration/debug.bats | Removed obvious comments |
| tests/integration/create.bats | Removed redundant comments |
| tests/integration/checkpoint.bats | Removed obvious comments and fixed duplicate run usage |
| tests/integration/cgroups.bats | Improved readability with heredoc and fixed comment |
| tests/integration/README.md | Updated documentation with better references |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This is the first part of tests/integration cleanup I wanted to do for quite some time.
Mostly some cleanups, a couple of real fixes.
High level overview:
Please see individual commits for details.